Tracing without Performance #2788

adinauer · 2023-06-14T20:15:32Z

📜 Description

Pass through / add sentry-trace and baggage even if performance is disabled.

💡 Motivation and Context

Closes #2768

💚 How did you test it?

wanna do some more e2e testing before merging

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

adinauer · 2023-06-14T20:16:36Z

@markushi and @marandaneto I'll leave some markers where I'd like your input. A full review would be very welcome as well.

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java

...ry-android-navigation/src/main/java/io/sentry/android/navigation/SentryNavigationListener.kt

marandaneto · 2023-06-15T14:07:05Z

sentry/src/main/java/io/sentry/util/TracingUtils.java

+        });
+  }
+
+  public static void traceIfAllowed(


M: I believe it'd be better if this just returns a boolean?
Executing a callback forces you to have Atomic operations in a few different places.

Hmm could change it to return the TracingHeaders instead of having a callback. With @Nullable it'd only be a simple null check on the calling side. If this returned a boolean we'd have to duplicate the extraction code everywhere.

Then returning TracingHeaders looks already better.

marandaneto · 2023-06-16T08:09:16Z

@markushi and @marandaneto I'll leave some markers where I'd like your input. A full review would be very welcome as well.

maybe @romtsn or @stefanosiano ? Markus is OOO.

marandaneto · 2023-06-16T08:11:11Z

@adinauer related to your questions/markers, I believe @cleptric can help? since it's not Android related, but rather how this feature should work, Also https://www.notion.so/sentry/Tracing-without-performance-efab307eb7f64e71a04f09dc72722530

The places you added a TODO are where a new transaction would be created (but performance is disabled), so in this case, you do nothing, right?
The important part is that if there's an outgoing request or something, you have to attach the propagation even if performance is disabled, and the propagation is already living in the Hub/Scope, IIRC.

adinauer · 2023-06-16T08:44:52Z

It's basically the following question and answer from #2768:

There's other places where we start transactions that don't originate from incoming requests but some user interaction / system call on Android. Do we create a new PropagationContext in all those spots where we'd create a transaction if performance was enabled?
Answer: If the button click (or similar) creates a new TraceID today when using performance, it should also create a new TraceID when performance is off, so a new PropagationContext

So in theory we should be starting a new trace there. I just wanted your input for Android specifics.

romtsn · 2023-06-16T09:04:13Z

With my (limited) understanding of tracing, I'd say we should start a new trace for all of the markers you left. When UI is involved, a User Interaction (whether navigation or click) usually marks the start of a new trace (e.g. you clicked "Save" button - then you make api/save request, you clicked Delete button - then you make api/delete request; it makes sense that these are two separate traces with two separate span hierarchies).

marandaneto · 2023-06-16T09:29:24Z

With my (limited) understanding of tracing, I'd say we should start a new trace for all of the markers you left. When UI is involved, a User Interaction (whether navigation or click) usually marks the start of a new trace (e.g. you clicked "Save" button - then you make api/save request, you clicked Delete button - then you make api/delete request; it makes sense that these are two separate traces with two separate span hierarchies).

Yes, exactly, what I meant by "you do nothing because performance is disabled" means, you don't start a transaction.
But you have to reset(or create?) the PropagationContext in the Hub/Scope.
I understood previously that when you create a Hub/Scope, the PropagationContext would be created automatically already, and you do nothing, but the use case above makes more sense.

…ting tests

github-actions · 2023-06-19T08:52:36Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against a360064

github-actions · 2023-06-19T09:08:43Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	340.02 ms	429.10 ms	89.08 ms
Size	1.72 MiB	2.29 MiB	574.41 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`dc67004`	273.86 ms	346.37 ms	72.51 ms
`9246ed4`	275.63 ms	321.31 ms	45.69 ms
`8820c5c`	330.60 ms	416.86 ms	86.26 ms
`9246ed4`	281.79 ms	352.08 ms	70.29 ms
`0310da5`	381.20 ms	404.50 ms	23.30 ms
`496bdfd`	272.86 ms	407.33 ms	134.48 ms
`496bdfd`	301.22 ms	343.96 ms	42.73 ms

App size

Revision	Plain	With Sentry	Diff
`dc67004`	1.72 MiB	2.28 MiB	573.45 KiB
`9246ed4`	1.72 MiB	2.28 MiB	572.22 KiB
`8820c5c`	1.72 MiB	2.28 MiB	571.82 KiB
`9246ed4`	1.72 MiB	2.28 MiB	572.22 KiB
`0310da5`	1.72 MiB	2.28 MiB	573.45 KiB
`496bdfd`	1.72 MiB	2.28 MiB	571.82 KiB
`496bdfd`	1.72 MiB	2.28 MiB	571.82 KiB

Previous results on branch: feat/tracing-without-performance

Startup times

Revision	Plain	With Sentry	Diff
`25cb71a`	322.37 ms	395.86 ms	73.49 ms
`d50b334`	299.84 ms	401.56 ms	101.72 ms
`0a8926a`	293.88 ms	333.60 ms	39.72 ms
`bfaa13c`	256.71 ms	313.62 ms	56.91 ms
`c4c8632`	295.86 ms	322.98 ms	27.12 ms
`9c8777f`	280.52 ms	370.46 ms	89.94 ms
`03de2a6`	281.98 ms	318.64 ms	36.66 ms
`1d85b7f`	303.18 ms	356.62 ms	53.44 ms
`6da169c`	295.60 ms	390.86 ms	95.26 ms
`8cfda4c`	355.00 ms	425.33 ms	70.33 ms

App size

Revision	Plain	With Sentry	Diff
`25cb71a`	1.72 MiB	2.28 MiB	572.59 KiB
`d50b334`	1.72 MiB	2.29 MiB	574.13 KiB
`0a8926a`	1.72 MiB	2.29 MiB	574.47 KiB
`bfaa13c`	1.72 MiB	2.29 MiB	574.47 KiB
`c4c8632`	1.72 MiB	2.29 MiB	574.15 KiB
`9c8777f`	1.72 MiB	2.29 MiB	574.16 KiB
`03de2a6`	1.72 MiB	2.29 MiB	574.47 KiB
`1d85b7f`	1.72 MiB	2.28 MiB	572.59 KiB
`6da169c`	1.72 MiB	2.29 MiB	574.39 KiB
`8cfda4c`	1.72 MiB	2.29 MiB	574.47 KiB

codecov · 2023-06-20T06:18:12Z

Codecov Report

Patch coverage: 77.75% and project coverage change: +0.10 🎉

Comparison is base (dc67004) 81.15% compared to head (a360064) 81.25%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2788      +/-   ##
============================================
+ Coverage     81.15%   81.25%   +0.10%     
- Complexity     4501     4560      +59     
============================================
  Files           348      350       +2     
  Lines         16685    16870     +185     
  Branches       2268     2274       +6     
============================================
+ Hits          13540    13708     +168     
- Misses         2198     2219      +21     
+ Partials        947      943       -4

Impacted Files	Coverage Δ
...y/spring/boot/jakarta/SentryAutoConfiguration.java	`97.18% <ø> (ø)`
...io/sentry/spring/boot/SentryAutoConfiguration.java	`97.18% <ø> (ø)`
...racing/SentrySpanClientHttpRequestInterceptor.java	`0.00% <0.00%> (ø)`
...racing/SentrySpanClientHttpRequestInterceptor.java	`0.00% <0.00%> (ø)`
sentry/src/main/java/io/sentry/HubAdapter.java	`95.58% <0.00%> (-4.42%)`	⬇️
sentry/src/main/java/io/sentry/IHub.java	`92.59% <ø> (ø)`
sentry/src/main/java/io/sentry/NoOpHub.java	`44.68% <0.00%> (-3.05%)`	⬇️
sentry/src/main/java/io/sentry/Sentry.java	`82.75% <0.00%> (-1.09%)`	⬇️
sentry/src/main/java/io/sentry/Hub.java	`75.17% <44.44%> (-1.34%)`	⬇️
...ry/src/main/java/io/sentry/PropagationContext.java	`74.50% <74.50%> (ø)`
... and 15 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

adinauer

@marandaneto @romtsn @markushi I've updated the PR now so should be good to review. Not sure about gesture listener and starting a new trace there.

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java

markushi

Nice one, already looks pretty complete to me 🚀 Left a few comments.

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java

sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt

sentry/src/main/java/io/sentry/Baggage.java

sentry/src/main/java/io/sentry/PropagationContext.java

sentry/src/main/java/io/sentry/Scope.java

sentry/src/main/java/io/sentry/util/TracingUtils.java

Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>

adinauer · 2023-06-27T07:54:52Z

@marandaneto any more changes you'd like to see before I merge this?

marandaneto · 2023-06-29T07:28:20Z

@marandaneto any more changes you'd like to see before I merge this?

PR is already approved, and all my comments were addressed, all good, thank you.

CHANGELOG.md

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java

…ityLifecycleIntegration.java Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java

romtsn · 2023-06-29T17:20:34Z

Made a couple of more suggestions as I realized we check for the breadcrumbs flag inside addBreadcrumb already, so we don't need it in onActivityResumed etc, but feel free to merge after that is addressed

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>

TwP

fdf5953

adinauer commented Jun 14, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java Outdated Show resolved Hide resolved

adinauer commented Jun 14, 2023

View reviewed changes

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java Outdated Show resolved Hide resolved

adinauer commented Jun 14, 2023

View reviewed changes

...ry-android-navigation/src/main/java/io/sentry/android/navigation/SentryNavigationListener.kt Outdated Show resolved Hide resolved

fix tests

6eba5bc

marandaneto reviewed Jun 15, 2023

View reviewed changes

adinauer added 3 commits June 19, 2023 09:20

also add headers for okhttp if no span is active; add tests; fix exis…

537b3fb

…ting tests

Change trace and traceIfAllowed to return instead of taking a callback

686eb4e

Merge branch 'main' into feat/tracing-without-performance

62a3b39

adinauer added 4 commits June 19, 2023 12:16

Start new trace in ActivityLifecycleIntegratin

4e850af

Also start a new trace in SentryNavigationListener

15ad792

Add tests for continueTrace in spring filters

3cb799c

Add changelog

5feaac9

Merge branch 'main' into feat/tracing-without-performance

2bf3e3e

adinauer commented Jun 20, 2023

View reviewed changes

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java Outdated Show resolved Hide resolved

adinauer added 2 commits June 20, 2023 10:06

Add test for TracingUtils

1b1f062

More tests

66e4bdb

markushi reviewed Jun 20, 2023

View reviewed changes

adinauer marked this pull request as ready for review June 20, 2023 12:57

adinauer requested review from romtsn and stefanosiano as code owners June 20, 2023 12:57

adinauer mentioned this pull request Jun 26, 2023

Remove code that set tracesSampleRate to 0.0 for Spring Boot if not set #2800

Merged

7 tasks

adinauer and others added 3 commits June 26, 2023 17:45

Extra variable

ad3d2a4

Apply suggestions from code review

7db620b

Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>

Mark PropagationContext internal

38956f1